cli: add buildtree checkout command#2129
Conversation
ownik
requested review from
BenjaminSchubert,
abderrahim,
cs-shadow,
gtristan and
juergbi
as code owners
ownik
force-pushed
the
master
branch
3 times, most recently
from
948c3f3 to
cb60d5b
Compare
|
@BenjaminSchubert @abderrahim @cs-shadow @gtristan @juergbi I've made changes to verify that buildtree exists in the cache, similar to the shell command. Could you please review, please? |
|
Looks like this is fixing #1822, please link the issue in your PR. Also please add a test for this behaviour. I'll try to review it today. |
|
A few things need to be considered in the design of this feature:
|
Add a new `bst buildtree checkout` command for exporting buildtree stored in artifacts. By default, the command checks out the buildtree. The `--buildroot` option can be used to export the entire buildroot sandbox instead. Fixed apache#1822
|
@abderrahim Thanks for the feedback! I've reworked the implementation into a new I've also added an integration test covering the new command and link the issue in PR. |
ownik
changed the title
|
@abderrahim Any chance to review this PR? |
abderrahim
left a comment
abderrahim
left a comment
There was a problem hiding this comment.
Thanks for the contributions. See my comments below.
| # self.targets contains a list of the loaded target objects | ||
| # if we specify --deps build, Stream._load() will return a list | ||
| # of build dependency objects, however, we need to prepare a sandbox | ||
| # with the target (which has had its appropriate dependencies loaded) | ||
| element: Element = self.targets[0] |
There was a problem hiding this comment.
Does this make sense for the build tree? Either we're checking out just the buildtree, and then dependencies don't make sense, or we check out the whole buildroot and it already contains the build dependencies.
Am I missing something?
| # try to pull them if they are not already cached. | ||
| # | ||
| self.query_cache(elements) | ||
| self._pull_missing_artifacts(elements) |
There was a problem hiding this comment.
I think we only need to pull the artifact for the element in question. No need for dependencies here.
as an aside, I wonder if we should try to override the pull-buildtrees configuration in this case: if we're pulling to extract a build tree, we should probably try to pull the buildtree even if we usually don't.
|
|
||
| try: | ||
| artifact = element._get_artifact() | ||
| virdir = artifact.get_buildroot() if buildroot else artifact.get_buildtree() |
There was a problem hiding this comment.
Another thing we need to be aware of (and also give a suggestion about to the user) is that not all elements have a build tree: only build elements do.
So it's not enough to check for the build root, we also need to check for the build tree. Then depending on the availability and the user request, we can suggest an alternative to the user.
| buildtree available | buildroot available | user requested | result |
|---|---|---|---|
| Y | Y | build tree | extract build tree |
| Y | Y | build root | extract build root |
| N | Y | build tree | error + suggest extract build root |
| N | Y | build root | extract build root |
| N | N | build tree | error |
| N | N | build root | error |
| project = str(datafiles) | ||
| element_name = "build-shell/buildtree.bst" | ||
| checkout = os.path.join(cli.directory, "checkout") | ||
| tar = os.path.join(cli.directory, "source-checkout.tar") |
There was a problem hiding this comment.
| tar = os.path.join(cli.directory, "source-checkout.tar") | |
| tar = os.path.join(cli.directory, "buildtree.tar") |
| @pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox") | ||
| def test_buildtree_checkout_fail(cli, datafiles): | ||
| project = str(datafiles) | ||
| element_name = "build-shell/buildtree.bst" |
There was a problem hiding this comment.
We should also test the buildtree of a script element (using build-shell/script.bst). This is the corner case I mentioned above where we have a build root but not a build tree.
2 participants
This PR adds a "--use-buildtrees" option to "bst artifacts checkout", similar to the existing option in "bst shell".
The goal is to make it possible to check out build artifacts from the sandbox build tree when it is available.
Would be okay with adding this functionality, or are there reasons not to expose it through "bst artifacts checkout"? Or is there another way to get build artifacts from sandbox now?